If possible, I'd love to change String to &str. Logically we can assume that this object owns the necessary string somehow, so we can just return a slice.

...and Option<&str>, and so on.

(&str, Option<&str>, Option<&str>)

Since we're using &str everywhere, we can delete the .to_string() calls here and save a bunch of heap allocations.

Oh wow yeah that's way cleaner. Not sure why I was using Strings everywhere.

If we were being purely pendantic, it might be nice to change .unwrap() to .expect("could not parse URL_PARSE"), but that's being a bit pendantic, and much of compose_yml is old enough that I hadn't started doing that yet myself. :-)

Either expect or unwrap is fine here, because this is a true assertion violation.

Also, we might add a couple of :? "no-capture" markers as follows:

^([^#]+)(?:#([^:]+)?(?::(.+))?)?$

I think those are in the right place. :-) This would also require changing the arguments to at below.

Aha, I didn't even know about no-capture, that's awesome.

This unwrap, on the other hand, looks like a possible runtime panic, because you're matching a complex regex against a user-provided string. And unwrap/expect are really meant to be assertions, not error-checks—except maybe in very short "scripts".

So we need to do one of two things here:

1. Provide an iron-clad argument that captures can never fail on any possible input, or
2. Redesign this class / API so that we can only construct a GitUrl with a valid URL.

The later might look like:

pub struct GitUrl {
   url: String, // Must be private to maintain invariants.
} 

impl GitUrl {
   fn new<S: Into<String>>(url: S) -> Result<GitUrl> {
       let url = GitUrl { url: url.into() };
       // Make sure our URL can always be parsed.
       url.parse_parts()?;
       Ok(url)
   }
}

Then later you can write:

fn parse_parts(&self) -> Result<(&str, Option<&str>, Option<&str>)> { ... }

pub fn subdirectory(&self) -> Option<&str> {
    let (_, _, subdir) = self.parse_parts()
        .expect("parse_parts failed on data that we already parsed once successfully");
    subdir
}

The nice thing about this is that it allows us to add other validity checks to new in the future as needed. Alternatively, we could keep the current API, but provide a comment explaining why parse_parts can never fail on any possible input.

Yeah I was conscious of the unwrap here. I'm pretty certain that it can't blow up though. As far as I can tell, the only way captures can fail is if the url is empty, or if it starts with a hash. And either of those cases would fail the the existing should_treat_as_url check.

Now that said, I can totally see a situation where someone might change the should_treat_as_url regex, without realising that there's an unwrap 70 lines down that depends on it.

Conclusion: I'll go with your suggestion and make new call parse_parts, so it definitely can't panic.

Just looking at your proposed snippet above, you've removed the should_treat_as_url check from new. I feel like we should keep that check there, seeing as it ensures Docker will accept the URL. The parse_parts regex will accept strings like "a#b:c", or even just "a", which is obviously not a URL.

Oh, yeah, definitely include whatever is already there in new. I wrote that new example from scratch.

Ah, ok, great.

Now that said, I can totally see a situation where someone might change the should_treat_as_url regex, without realising that there's an unwrap 70 lines down that depends on it.

Yeah, a good rule of thumb is that you shouldn't call unwrap in a doubtful situation like this. At a minimum, there should be an expect and a explanation of why this really is an assertion, and not a run-time error condition.

Ok I'm really close with this, just struggling with the final piece. Right now I have this:

    fn parse_parts(&self) -> Result<(&str, Option<&str>, Option<&str>)> {
        lazy_static! {
            static ref URL_PARSE: Regex = Regex::new(r#"^([^#]+)(?:#([^:]+)?(?::(.+))?)?$"#)
                .expect("Could not parse regex URL_PARSE");
        }
        let captures = URL_PARSE.captures(&self.url).unwrap();
        Ok((
            captures.at(1).unwrap(),
            captures.at(2),
            captures.at(3),
        ))
    }

I've wrapped the return type in Result, and I'm handling the Result everywhere that calls this, but as you can see I still have the unwrap. I tried to replace unwrap() withok_or(())? to convert the Option into a Result but couldn't get the types to work. Is it because the Result here is an error-chain thing, not a regular Result?

Yeah, you need to construct our local Error type here, which you can actually do with a string and .into(). Use ok_or_else(|| -> Error { format!(...).into() }) and it will construct an error for you.

This is great, but let's also add a test case for format!("{}{}", url, ":myfolder")), specifying how that should be parsed.

Looking at the Docker docs, I don't think that's a valid URL? Also, the parsing as it's currently written relies on the hash as a nice, easy-to-spot delimeter. A lone colon is harder to split on, because it could be for a port number. 🤔

Oh, sorry! I should have been more clear. I want to make sure format!("{}{}", url, ":myfolder")) isn't parsed as a subdir. It's a suspicious input, where the implementation might plausibly fail, and I want to make sure that we interpret it correctly.

Does this make sense?

This is an amazingly good choice of API. Thank you for figuring this out. Maybe it might be clearer if we named it without_repository_subdirectory?

Let's add a comment explaining the invariant we're trying to enforce (otherwise this might be a bit mysterious):

// Invariant: `parse_parts` should succeed for all `GitUrl` values after construction.

Let's make that error: format!("could not parse URL {:?}", self.url).into().